[BOLT] Build CFG for non-simple functions in aggregation mode #128944
+5
−5
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Created using spr 1.3.4
aaupov
requested review from
ayermolo,
dcci,
maksfb,
rafaelauler and
yota9
as code owners
Member
|
@llvm/pr-subscribers-bolt Author: Amir Ayupov (aaupov) ChangesExtend #128253 by allowing all non-simple functions be processed in Test Plan: entry-point-fallthru.s Full diff: https://github.com/llvm/llvm-project/pull/128944.diff 3 Files Affected:
diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp
index ff5eb5cf6e1eb..161733d75a2ab 100644
--- a/bolt/lib/Core/BinaryFunction.cpp
+++ b/bolt/lib/Core/BinaryFunction.cpp
@@ -1754,8 +1754,7 @@ void BinaryFunction::postProcessEntryPoints() {
// In non-relocation mode there's potentially an external undetectable
// reference to the entry point and hence we cannot move this entry
// point. Optimizing without moving could be difficult.
- // In aggregation, register any known entry points for CFG construction.
- if (!BC.HasRelocations && !opts::AggregateOnly)
+ if (!BC.HasRelocations)
setSimple(false);
const uint32_t Offset = KV.first;
@@ -2084,7 +2083,7 @@ void BinaryFunction::recomputeLandingPads() {
Error BinaryFunction::buildCFG(MCPlusBuilder::AllocatorIdTy AllocatorId) {
auto &MIB = BC.MIB;
- if (!isSimple()) {
+ if (!isSimple() && !opts::AggregateOnly) {
assert(!BC.HasRelocations &&
"cannot process file with non-simple function in relocs mode");
return createNonFatalBOLTError("");
diff --git a/bolt/lib/Profile/DataAggregator.cpp b/bolt/lib/Profile/DataAggregator.cpp
index d20626bd5062f..dc0f91030bc18 100644
--- a/bolt/lib/Profile/DataAggregator.cpp
+++ b/bolt/lib/Profile/DataAggregator.cpp
@@ -871,7 +871,7 @@ DataAggregator::getFallthroughsInTrace(BinaryFunction &BF,
BinaryContext &BC = BF.getBinaryContext();
- if (!BF.isSimple())
+ if (BF.empty())
return std::nullopt;
assert(BF.hasCFG() && "can only record traces in CFG state");
diff --git a/bolt/lib/Rewrite/RewriteInstance.cpp b/bolt/lib/Rewrite/RewriteInstance.cpp
index 70a9f084f009b..ae3e0b9ddce38 100644
--- a/bolt/lib/Rewrite/RewriteInstance.cpp
+++ b/bolt/lib/Rewrite/RewriteInstance.cpp
@@ -3438,7 +3438,8 @@ void RewriteInstance::buildFunctionsCFG() {
};
ParallelUtilities::PredicateTy SkipPredicate = [&](const BinaryFunction &BF) {
- return !shouldDisassemble(BF) || !BF.isSimple();
+ // Construct CFG for non-simple functions in aggregation mode.
+ return !(shouldDisassemble(BF) && (BF.isSimple() || opts::AggregateOnly));
};
ParallelUtilities::runOnEachFunctionWithUniqueAllocId(
|
Contributor
|
I think we need to clearly mark functions that may have incorrect basic block boundaries. Otherwise LGTM. |
Contributor
Author
What kind of marking do you have in mind, a new BinaryFunction attribute e.g. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extend #128253 by allowing all non-simple functions be processed in
aggregation mode, not just those with multiple entry points.
The CFG is needed for
getFallthroughsInTraceto convert profile intoYAML/fdata. We're primarily interested in basic block boundaries, with
some of them could be incorrectly identified for non-simple functions.
However, this is benign in aggregation mode, in the worst case resulting
in stale profile for such functions.
Test Plan: entry-point-fallthru.s